-
Notifications
You must be signed in to change notification settings - Fork 490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: MetadataBlock refactoring #9932
base: develop
Are you sure you want to change the base?
WIP: MetadataBlock refactoring #9932
Conversation
Moved MetadataBlock related code from DataverseService to MetadataBlockService. Further, its removing duplicate code.
src/main/java/edu/harvard/iq/dataverse/MetadataBlockConverter.java
Outdated
Show resolved
Hide resolved
…ldDefaultValue` from codebase.
@qqmyers: Initial I started with the simple refactoring of the MetadataBlock. While digging through the code I found some unused code blocks related to Datasets. I removed those in this MR as well. |
I did check with @sekmiller and we agree it's certainly possible that some of the entities/tables are not used. They may have been from early experiments and never removed. |
This issue is related and we could probably mark this PR as closing it: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad I am not alone in the marathon for cleaner code. Thanks for your efforts!
I would like to prevent that these changes introduce new issues, like incomplete javadoc comments, so could you have a look at them?
* @param mdb | ||
* @return | ||
*/ | ||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add the @deprecated
tag to the javadoc and provide a reason in the @Deprecated
annotation, according to Sonarcloud.
/** | ||
* The list of controlled vocabulary terms that may be used as values for | ||
* fields of this field type. | ||
* If the definition of the DatasetFieldType includes a definition for a controlled Vocabulary, i.e. a list of allowed values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a complete sentence – I expect another part starting with "then", because of "If".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to complete the javadoc tags if you add them. For deprecated code, could you add @deprecated
to the javadoc to explain why it is deprecated? Sonarcloud rule Java:S1123 explains why you should do this.
I agree, @pdurbin! |
@johannes-darms are you still interested in this? This refactoring hasn't been a priority for us. We have 107 open pull requests and I'm looking for some we can close. Maybe a "soft close" since we could always open it again if it becomes a priority. Please let us know. Thanks. |
If you are still interested in this PR, can you please merge and resolve any merge conflicts with the latest from develop? If so, we can prioritize reviewing and QAing the changes. If we don’t hear from you by May 22, 2024, we’ll go ahead and close this PR (it can always be reopened after that date, if there is still interest). |
I'm working on it. I hope I can deduct some time in may to this PR. |
What this PR does / why we need it:
Refactors code related to MetadataBlocks. Removes duplicated code and moves MetadataBlock related code from the DataverseService to the responsible MetadataBlockService.
The changed methods of DataverseService still exist (are not longer used according to IDEA) are marked as deprecated and facade to the MetadataBlockService implementation.
Furthermore, this removed the unused
DatasetDistributor
,DatasetKeyword
,DatasetRelMaterial
,DatasetTopicClass
,DatasetVersionDatasetUserId
andDatasetFieldDefaultValue
from the codebase.Which issue(s) this PR closes:
Closes #
Special notes for your reviewer: Just moved and removed duplicate code paths.
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here: No
Is there a release notes update needed for this change?: No
Additional documentation: